XML Namespace Declarations for XPath Expressions#2253
Conversation
WalkthroughAdds XML namespace configuration and parsing support, refactors expression APIs to use an Interceptor context via InterceptorAdapter, introduces a hardened XML parser and XmlConfig/Namespaces, updates many interceptors to propagate XmlConfig, and adds namespace-aware tests and examples. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Interceptor
participant InterceptorAdapter
participant ExchangeExpression
participant XPathExpr
participant XmlConfig
Client->>Interceptor: init(Router)
Interceptor->>Interceptor: setXmlConfig(XmlConfig)
Interceptor->>ExchangeExpression: expression(new InterceptorAdapter(router, xmlConfig), Language, expr)
ExchangeExpression->>XPathExpr: new XPathExchangeExpression(interceptor, xpath)
XPathExpr->>InterceptorAdapter: request XmlConfig / NamespaceContext
InterceptorAdapter->>XmlConfig: getNamespaces()
XmlConfig-->>XPathExpr: NamespaceContext
Client->>Interceptor: handleRequest(Exchange)
Interceptor->>ExchangeExpression: evaluate(exchange)
ExchangeExpression->>XPathExpr: evaluate with NamespaceContext
XPathExpr-->>Interceptor: result (namespace-aware)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (8)
distribution/conf/simple.yaml (1)
1-29: Consider adding security definitions for API hardening.The OpenAPI specification lacks security scheme definitions. While this may be acceptable for an example configuration file, best practices recommend defining authentication/authorization mechanisms.
If this configuration is meant to be production-ready or serves as a template, consider adding security schemes:
openapi: 3.1.0 info: title: Simple Parameters API version: "1.0.0" +security: + - bearerAuth: [] servers: - url: http://localhost +components: + securitySchemes: + bearerAuth: + type: http + scheme: bearerIf this is purely an example/test file, this can be safely ignored.
distribution/examples/xml/namespaces/membrane.cmd (1)
9-9: Consider removing redundant root check.Line 9 checks if
%dir%equals%SystemDrive%\, but this is redundant. Line 7 already properly detects when the root is reached by checking if the parent equals the current directory (which happens at any root, regardless of drive). Additionally, this check only works for the system drive and would fail to detect roots on other drives likeD:\.Apply this diff to remove the redundant check:
set "dir=%next%" -if "%dir%"=="%SystemDrive%\" goto notfound goto loopdistribution/examples/xml/namespaces/README.md (1)
12-14: Specify language for the fenced code block.The fenced code block should specify
bashorshellas the language identifier for proper syntax highlighting and consistency with markdown best practices.Apply this diff:
- Send: - ``` + ```bash curl -d @person.xml localhost:2000 ```core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyExchangeExpression.java (1)
38-45: Guard against missing router before compiling the script.If an expression is instantiated before the interceptor is fully wired (e.g., tests creating an InterceptorAdapter without calling
init()),interceptor.getRouter()will returnnull, leading to a lateNullPointerExceptionwhen Groovy compilation tries to access the background initializer. It would be safer to fail fast with aConfigurationException.- this.router = interceptor.getRouter(); + Router router = interceptor.getRouter(); + if (router == null) { + throw new ConfigurationException("Cannot compile Groovy script without an attached router."); + } + this.router = router;core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java (2)
84-86: Consider preserving exception cause for debugging.Suppressing the stack trace (line 85) may hinder troubleshooting in production environments. While the message is preserved, the underlying cause and stack trace can be valuable for diagnosing parse failures.
If you want to avoid verbose stack traces in logs, consider preserving the cause while keeping the message concise:
- throw new XmlParseException("Could not parse XML document: " + e.getMessage()); // No stacktrace needed + throw new XmlParseException("Could not parse XML document: " + e.getMessage(), e);Then handle stack trace logging at the caller level based on log verbosity settings.
61-61: Document why UnsupportedOperationException is ignored.The empty catch block for
UnsupportedOperationExceptionat line 61 silently swallows the exception. While this may be intentional for parsers that don't support XInclude, adding a comment would clarify the intent.- try { f.setXIncludeAware(false); } catch (UnsupportedOperationException ignore) {} + // Some parsers don't support XInclude configuration; ignore if not available + try { f.setXIncludeAware(false); } catch (UnsupportedOperationException ignore) {}core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
36-51: Consider parser initialization pattern.The static
parserfield is initialized in the constructor (line 50), which will execute once per instance creation. SinceHardenedXmlParser.getInstance()returns a singleton, this is redundant work.Initialize the parser at the class level instead:
- private static XmlParser parser; + private static final XmlParser parser = HardenedXmlParser.getInstance(); private Namespaces namespaces; // Let all expressions share the same XPathFactory. private static final XPathFactory factory = XPathFactory.newInstance(); public XPathExchangeExpression(Interceptor interceptor, String xpath) { super(xpath); if (interceptor instanceof XMLNamespaceSupport xns) { namespaces = xns.getNamespaces(); } - - parser = HardenedXmlParser.getInstance(); }distribution/examples/xml/namespaces/proxies.xml (1)
1-54: Update the namespace example README to document the namespace usage pattern.The existing README at
distribution/examples/xml/namespaces/README.mdis too brief. While it mentions "How to declare and use namespaces with setProperty, if, ...", it provides no actual explanation. Expand it to document:
- How to declare namespaces with an
idattribute (the<namespaces id="ns">block)- How to reference them in interceptors using
<spring:ref bean="ns"/>- Which interceptors support namespace declarations (rateLimiter, log, setProperty, setHeader, if, target)
Currently it just says "Take a look at the proxies.xml file," which leaves users to reverse-engineer the pattern from the example alone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
core/src/main/java/com/predic8/membrane/core/http/Request.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/XMLNamespaceSupport.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/ForInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractExchangeExpressionInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/Namespaces.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java(4 hunks)core/src/main/java/com/predic8/membrane/core/lang/groovy/GroovyExchangeExpression.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java(4 hunks)core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java(4 hunks)core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/WSDLUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/parser/XmlParseException.java(1 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/parser/XmlParser.java(1 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/lang/NamespacesTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/AbstractExchangeExpressionTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/TemplateExchangeExpressionTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpressionTest.java(3 hunks)core/src/test/java/com/predic8/membrane/core/util/XMLUtilTest.java(1 hunks)distribution/conf/apis.yaml(1 hunks)distribution/conf/log4j2.xml(1 hunks)distribution/conf/simple.yaml(1 hunks)distribution/examples/templating/json/README.md(2 hunks)distribution/examples/xml/namespaces/README.md(1 hunks)distribution/examples/xml/namespaces/membrane.cmd(1 hunks)distribution/examples/xml/namespaces/membrane.sh(1 hunks)distribution/examples/xml/namespaces/person.xml(1 hunks)distribution/examples/xml/namespaces/proxies.xml(1 hunks)distribution/examples/xml/namespaces/requests.http(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/VersioningSoapXsltExampleTest.java(2 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java(1 hunks)docs/ROADMAP.md(2 hunks)
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-07-02T18:05:47.059Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1947
File: distribution/tutorials/soap/membrane.sh:17-22
Timestamp: 2025-07-02T18:05:47.059Z
Learning: When using environment variable shortcuts like `$MEMBRANE_HOME` in shell scripts that look for installation directories, validate that required files (like `starter.jar` for Membrane) exist in the candidate directory before returning it. This prevents hard-to-diagnose runtime errors when the environment variable points to an invalid or stale location.
Applied to files:
distribution/examples/xml/namespaces/membrane.shdistribution/examples/xml/namespaces/membrane.cmd
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) and XMLBeautifier.parse(Reader) both declare "throws Exception" in their signatures, not XMLStreamException specifically. Catching IOException is appropriate for handling IO errors during XML parsing with XMLBeautifier.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/xml/parser/XmlParseException.java
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) throws IOException specifically, not Exception. It internally wraps any XMLStreamException or other parsing exceptions in an IOException. Therefore, catching IOException in TextUtil.formatXML is the correct approach for handling XML parsing errors from InputStream.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/xml/parser/XmlParseException.java
📚 Learning: 2025-07-28T16:28:17.849Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1977
File: core/src/main/java/com/predic8/membrane/core/interceptor/opentelemetry/OpenTelemetryInterceptor.java:107-109
Timestamp: 2025-07-28T16:28:17.849Z
Learning: In Java exception handling for OpenTelemetry body logging, use log.debug() level with ExceptionUtil.concatMessageAndCauseMessages(e) to log body reading failures. This prevents cluttering info logs with potentially common stream reading issues while still providing comprehensive error information when debug logging is enabled.
Applied to files:
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.javacore/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java
📚 Learning: 2025-07-11T14:18:23.761Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1977
File: core/src/main/java/com/predic8/membrane/core/interceptor/opentelemetry/OpenTelemetryInterceptor.java:107-109
Timestamp: 2025-07-11T14:18:23.761Z
Learning: In Java exception handling, use ExceptionUtil.concatMessageAndCauseMessages(e) instead of e.getMessage() or e.getCause().getMessage() to get a complete view of the exception chain. This utility method safely concatenates all exception messages in the chain with " caused by: " separator, providing comprehensive error information for debugging without risk of NullPointerException.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java
🧬 Code graph analysis (13)
core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java (1)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
XMLUtil(30-85)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractExchangeExpressionInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
TemplateExchangeExpression(29-166)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (2)
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java (1)
HardenedXmlParser(30-88)core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
XMLUtil(30-85)
core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (4)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/Namespaces.java (2)
MCElement(24-103)MCElement(47-73)core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (1)
MCElement(101-248)core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
MCElement(47-110)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java (1)
MCElement(41-149)
core/src/main/java/com/predic8/membrane/core/util/WSDLUtil.java (1)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
XMLUtil(30-85)
core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (1)
core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
TemplateExchangeExpression(29-166)
core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-369)
core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
XMLUtil(30-85)
core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
ProblemDetails(36-414)
core/src/test/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpressionTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-369)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/Namespaces.java (5)
core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (1)
MCElement(101-248)core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
MCElement(47-110)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java (1)
MCElement(41-149)core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (1)
MCElement(29-93)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java (2)
MCElement(43-252)MCElement(226-238)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.java (2)
core/src/main/java/com/predic8/membrane/core/exceptions/ProblemDetails.java (1)
ProblemDetails(36-414)core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java (1)
ExceptionUtil(20-40)
core/src/test/java/com/predic8/membrane/core/interceptor/lang/NamespacesTest.java (1)
core/src/main/java/com/predic8/membrane/core/util/CollectionsUtil.java (1)
CollectionsUtil(23-70)
🪛 Checkov (3.2.334)
distribution/conf/simple.yaml
[high] 1-29: Ensure that the global security field has rules defined
(CKV_OPENAPI_4)
[high] 1-29: Ensure that security operations is not empty.
(CKV_OPENAPI_5)
[medium] 16-20: Ensure that arrays have a maximum number of items
(CKV_OPENAPI_21)
🪛 markdownlint-cli2 (0.18.1)
distribution/examples/xml/namespaces/README.md
12-12: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Shellcheck (0.11.0)
distribution/examples/xml/namespaces/membrane.sh
[warning] 8-8: Remove space after = if trying to assign a value (for empty string, use var='' ... ).
(SC1007)
🔇 Additional comments (52)
distribution/examples/templating/json/README.md (1)
15-15: Step numbering shift is appropriate given the new step 2.The renumbering of steps 3→3, 4→4, and 5→5 follows logically from introducing step 2 (viewing
proxies.xmlbefore execution). The documentation now guides users through understanding the configuration before running the example, which is a sensible pedagogical improvement.Also applies to: 17-17, 29-29
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
314-318: LGTM! Clean implementation following the established pattern.The
xml()method correctly mirrors thejson()method's approach, setting the body content and appropriate Content-Type header. This provides a convenient way to build XML requests and aligns well with the PR's objective of enhancing XML namespace support.distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (2)
1-13: License header is correct.Standard Apache License 2.0 header is properly formatted.
18-23: Code structure is correct and verified.The example directory
distribution/examples/xml/namespaces/exists at the expected location, and thegetExampleDirName()method correctly references it. No changes needed.distribution/examples/xml/namespaces/membrane.sh (2)
8-8: Shellcheck warning is a false positive.The syntax
CDPATH=is intentional and correct. It temporarily sets CDPATH to empty for just that command to prevent CDPATH pollution when computing the script's directory. This is a well-known shell idiom.
10-18: LGTM! Correct implementation with proper validation.The upward directory traversal correctly validates that both
starter.jarandscripts/run-membrane.shexist before using a directory, which prevents hard-to-diagnose runtime errors. This follows the established learning. Based on learningsdistribution/examples/xml/namespaces/membrane.cmd (1)
5-5: LGTM! Proper validation of required files.The script correctly validates that both
starter.jarandscripts\run-membrane.cmdexist before using the directory, which prevents hard-to-diagnose runtime errors. Based on learningsdistribution/conf/log4j2.xml (1)
12-14: INFO level aligns with production defaults.Keeps the HTTP client transport logs quieter unless problems arise.
core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java (1)
97-97: Passing the interceptor instance matches the new factory contract.Ensures namespace-aware evaluation has access to this interceptor’s context.
docs/ROADMAP.md (1)
34-34: Appreciate the explicit SessionManagerTest TODO.Having it on the roadmap makes the migration visible.
distribution/src/test/java/com/predic8/membrane/examples/withinternet/test/VersioningSoapXsltExampleTest.java (1)
57-58: Conditional logging tightens diagnostics.
.log().ifValidationFails(ALL)keeps green runs quiet while preserving failure detail.distribution/examples/xml/namespaces/person.xml (1)
1-6: Nice, concise namespace example.This will help users see prefix declarations in practice.
core/src/test/java/com/predic8/membrane/core/interceptor/lang/NamespacesTest.java (1)
38-41: Test covers the essential namespace lookups.Great to see
getNamespaceURI,getPrefix, andgetPrefixesall asserted.core/src/main/java/com/predic8/membrane/core/interceptor/XMLNamespaceSupport.java (1)
1-9: Clean contract for namespace-aware interceptors.This keeps namespace propagation explicit and easy to implement.
core/src/main/java/com/predic8/membrane/core/util/xml/parser/XmlParseException.java (1)
1-34: LGTM!Clean unchecked exception implementation with clear purpose documented in the javadoc. The wrapping of SAXException and IOException as a RuntimeException simplifies error handling for callers.
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractSetterInterceptor.java (2)
54-56: Root cause extraction improves error messages.Extracting the root cause provides clearer error messages by avoiding wrapper exception noise. The use of
ExceptionUtil.getRootCause()aligns with the pattern used elsewhere in the codebase.
56-56: Verify that info-level logging is appropriate for expression evaluation failures.The logging level changed from
errortoinfo. While this reduces log noise, expression evaluation failures might warrant higher visibility in production environments, especially whenfailOnErroris true and requests are being aborted.Consider whether these failures should remain at
errorlevel whenfailOnError=trueto ensure operational visibility of rejected requests.distribution/conf/apis.yaml (1)
1-509: LGTM!The configuration demonstrates comprehensive API gateway capabilities with diverse interceptor usage patterns including logging, scripting (Groovy/JavaScript), templating, authentication, and protection mechanisms. The structure is clear and follows the expected YAML schema.
core/src/main/java/com/predic8/membrane/core/util/SOAPUtil.java (1)
32-32: LGTM!The static import path update reflects the package relocation of
XMLUtilto thexmlsubpackage. No functional changes.distribution/examples/xml/namespaces/requests.http (1)
1-9: LGTM!Clear example demonstrating XML namespace usage with multiple namespace prefixes (
per:andns1:). The request structure is well-formed.core/src/main/java/com/predic8/membrane/core/interceptor/soap/SampleSoapServiceInterceptor.java (1)
37-37: LGTM!The static import path update reflects the package relocation of
XMLUtilto thexmlsubpackage. No functional changes.core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java (2)
119-120: LGTM!Test correctly updated to use
InterceptorAdapterwrapper, aligning with the refactoredExchangeExpression.newInstance()API signature. The functional test logic remains unchanged.
124-125: LGTM!Test correctly updated to use
InterceptorAdapterwrapper, consistent with the API changes throughout the PR.core/src/main/java/com/predic8/membrane/core/util/WSDLUtil.java (1)
27-27: LGTM!The import path update correctly reflects the relocation of XMLUtil to the
util.xmlpackage.core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
14-14: LGTM!The package relocation to
util.xmlimproves code organization by grouping XML-related utilities together.core/src/test/java/com/predic8/membrane/core/util/XMLUtilTest.java (1)
16-16: LGTM!The wildcard import correctly references the relocated XMLUtil package.
core/src/test/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpressionTest.java (1)
118-138: Good test coverage for namespace handling.The new test correctly validates XPath evaluation with namespaced XML using the
local-name()function. This provides essential coverage for the XML namespace support feature.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java (2)
24-25: LGTM!The import consolidation using wildcards is consistent with the codebase style.
36-36: LGTM!The refactor to use
InterceptorAdapteraligns with the broader PR changes and enables proper namespace context propagation for expression evaluation.core/src/test/java/com/predic8/membrane/core/lang/AbstractExchangeExpressionTest.java (1)
67-80: LGTM!The test helper methods have been consistently updated to use
InterceptorAdapter, maintaining uniform test infrastructure while supporting the new namespace-aware expression evaluation.core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java (4)
23-25: LGTM!The wildcard imports consolidate related classes and align with the codebase style.
60-60: LGTM!The
Namespacesfield addition enables XML namespace support for API expressions.
86-86: LGTM!The
ExchangeExpressioninitialization correctly passes the namespaces viaInterceptorAdapter, enabling namespace-aware expression evaluation for API test scripts.
240-251: LGTM!The namespace getter/setter methods are properly annotated with
MCChildElement, enabling XML configuration of namespaces for XPath expressions. The Javadoc clearly describes the purpose.core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractExchangeExpressionInterceptor.java (1)
29-31: Good call moving to an interceptor-supplied context.Passing
thiskeeps the ExchangeExpression factory aligned with the new namespace-aware API surface. Nicely done.core/src/main/java/com/predic8/membrane/core/util/xml/parser/XmlParser.java (1)
20-34: Clean, focused parser contract.The interface and javadoc clearly document the hardened, thread-safe expectations—this will make future implementations straightforward.
core/src/main/java/com/predic8/membrane/core/interceptor/flow/ForInterceptor.java (1)
55-63: Alignment with interceptor-based expressions looks good.Switching to
ExchangeExpression.newInstance(this, …)keeps the namespace-aware plumbing consistent across flow interceptors.core/src/test/java/com/predic8/membrane/core/lang/TemplateExchangeExpressionTest.java (1)
45-70: Solid coverage update.Using
InterceptorAdapterin the tests mirrors the production wiring and protects the new constructor signature; thanks for keeping the fixtures in sync.core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java (1)
42-148: Namespace plumbing looks consistent.Implementing
XMLNamespaceSupportdirectly inIfInterceptorand feeding the interceptor instance into the expression factory should make XPath use much easier to configure.core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (2)
102-119: LGTM! Clean integration of namespace support.The
Targetclass correctly implementsXMLNamespaceSupportand usesInterceptorAdapterto pass namespace context during expression initialization. This aligns well with the broader namespace support framework introduced in this PR.
240-247: LGTM! Proper namespace accessor implementation.The getter/setter methods for namespaces follow the established pattern and are properly annotated with
@MCChildElementfor XML configuration support.core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (2)
48-58: LGTM! Namespace support properly integrated.The implementation correctly adopts
XMLNamespaceSupportand usesInterceptorAdapterto pass namespace context during expression initialization, consistent with the broader namespace support framework.
98-109: LGTM! Well-documented namespace configuration.The Javadoc clearly describes the purpose of the namespace declarations for XPath expressions, and the annotation is correctly configured.
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java (1)
25-58: LGTM! Base class properly extended with namespace support.The abstract class now provides namespace support to all language interceptors through the
XMLNamespaceSupportinterface. The implementation is clean and allows subclasses to inherit this functionality.core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (2)
30-41: LGTM! Namespace support correctly implemented.The
Caseclass properly adopts theXMLNamespaceSupportinterface and usesInterceptorAdapterto provide namespace context during expression evaluation, consistent with the framework pattern.
81-92: LGTM! Namespace accessors properly implemented.The setter/getter methods follow the established pattern with appropriate annotations and documentation.
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (2)
88-96: LGTM! Thread-safe XPath evaluation with namespace support.The implementation correctly creates a fresh
XPathinstance for each evaluation (thread-safe) and properly applies the namespace context when available. The use ofHardenedXmlParserensures XXE protection.
43-48: Clean namespace extraction from interceptor.The pattern of checking if the interceptor implements
XMLNamespaceSupportand extracting namespaces is clean and follows the framework design well.core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java (2)
68-93: Well-designed adapter for backward compatibility.The
InterceptorAdapterinner class provides a clean bridge for contexts where anInterceptoris not available, wrapping aRouterand optionalNamespaces. The two-constructor pattern (with/without namespaces) is convenient and follows good design practices.
52-67: Breaking change successfully propagated throughout codebase.All 16 usages of
ExchangeExpression.newInstance()have been updated to use the newInterceptorparameter instead ofRouter. Callers properly provide either:
- Direct interceptor instances (e.g.,
thisin interceptor classes)- Wrapped router via
InterceptorAdapter(in non-interceptor contexts)nullfor languages that don't require an interceptor (SPEL, JSONPATH)No remaining calls with the old
Routersignature exist. The migration is complete and consistent.core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java (1)
49-55: Interceptor-scoped expression init looks solid.Passing
thisintoExchangeExpression.newInstancekeeps the namespace context attached to the interceptor, aligning with the new language/ns plumbing. Looks good from my side.core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
95-104: Nested expressions now inherit namespace context.Threading the interceptor through
${…}token instantiation prevents namespace loss in embedded expressions. Nice work.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
52-69: Ensure parsing exceptions are properly handled.The
evaluatemethod catchesXPathExpressionExceptionon line 65, butparser.parse()on line 93 can throwXmlParseException(a runtime exception), which would bypass this catch block and propagate uncaught. This breaks the clean error reporting established byExchangeExpressionException.Apply this diff to handle parsing failures consistently:
try { if (Boolean.class.isAssignableFrom(type)) { return type.cast( evalutateAndCast(msg, BOOLEAN)); } if (String.class.isAssignableFrom(type)) { return type.cast(evalutateAndCast(msg, STRING)); } if (Object.class.isAssignableFrom(type)) { return type.cast( evaluateAndCastToObject( msg)); } throw new RuntimeException("Should not Happen!"); - } catch (XPathExpressionException xee) { + } catch (XPathExpressionException | XmlParseException xee) { throw new ExchangeExpressionException(expression,xee) .body(msg.getBodyAsStringDecoded()) .stacktrace(false); }
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java (1)
16-18: Remove unused imports.The imports
com.predic8.membrane.core.util.xml.parser.*andjava.util.*are not referenced in this file.Apply this diff:
-import com.predic8.membrane.core.util.xml.parser.*; - -import java.util.*; - public class ExceptionUtil {core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java (1)
17-18: Consider using explicit imports instead of wildcards.Wildcard imports can obscure which classes are actually used and may lead to naming conflicts as the packages evolve. Explicit imports improve code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/Namespaces.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java(1 hunks)distribution/examples/xml/namespaces/README.md(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/lang/Namespaces.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-11T14:18:23.761Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1977
File: core/src/main/java/com/predic8/membrane/core/interceptor/opentelemetry/OpenTelemetryInterceptor.java:107-109
Timestamp: 2025-07-11T14:18:23.761Z
Learning: In Java exception handling, use ExceptionUtil.concatMessageAndCauseMessages(e) instead of e.getMessage() or e.getCause().getMessage() to get a complete view of the exception chain. This utility method safely concatenates all exception messages in the chain with " caused by: " separator, providing comprehensive error information for debugging without risk of NullPointerException.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java
📚 Learning: 2025-07-28T16:28:17.849Z
Learnt from: rrayst
Repo: membrane/api-gateway PR: 1977
File: core/src/main/java/com/predic8/membrane/core/interceptor/opentelemetry/OpenTelemetryInterceptor.java:107-109
Timestamp: 2025-07-28T16:28:17.849Z
Learning: In Java exception handling for OpenTelemetry body logging, use log.debug() level with ExceptionUtil.concatMessageAndCauseMessages(e) to log body reading failures. This prevents cluttering info logs with potentially common stream reading issues while still providing comprehensive error information when debug logging is enabled.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java
🧬 Code graph analysis (1)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (2)
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java (1)
HardenedXmlParser(30-92)core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
XMLUtil(30-85)
🪛 LanguageTool
distribution/examples/xml/namespaces/README.md
[grammar] ~4-~4: Ensure spelling is correct
Context: ...clare and use namespaces. The namespace decarations can be used in: - setProperty - setHea...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (4)
distribution/examples/xml/namespaces/README.md (1)
16-27: Clear and well-structured documentation.The running instructions are easy to follow, with step-by-step guidance referencing the relevant files (person.xml, proxies.xml). The note about requests.http availability adds helpful context.
core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java (1)
34-42: LGTM! Null guard properly addresses the previous concern.The null check on line 35 prevents NPE when callers pass
nullorexception.getCause()returnsnull. The loop correctly traverses the cause chain while avoiding cycles.core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
86-94: LGTM! Thread-safe XPath evaluation with namespace support.Creating a fresh
XPathinstance per evaluation (line 87) correctly avoids thread-safety issues. The conditional namespace context binding (lines 89-91) integrates cleanly with the newXMLNamespaceSupportinfrastructure.core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java (1)
47-58: LGTM! Clean integration of XML namespace support.The
@MCChildElement(allowForeign = true)annotation on line 51 correctly allows namespace declarations in XML configuration, and the getter/setter pair properly implements theXMLNamespaceSupportinterface.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java (1)
89-93: Preserve the original parse exception.Re-throwing without the cause drops the stack trace and repeats a previously flagged issue. Please chain the caught exception so downstream diagnostics aren’t blind.
- } catch (Exception e) { - throw new XmlParseException("Could not parse XML document: " + e.getMessage()); // No stacktrace needed + } catch (Exception e) { + throw new XmlParseException("Could not parse XML document: " + e.getMessage(), e);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/main/java/com/predic8/membrane/core/interceptor/XMLNamespaceSupport.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java(2 hunks)core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java(1 hunks)distribution/examples/xml/namespaces/README.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/examples/xml/namespaces/README.md
- core/src/main/java/com/predic8/membrane/core/util/ExceptionUtil.java
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) and XMLBeautifier.parse(Reader) both declare "throws Exception" in their signatures, not XMLStreamException specifically. Catching IOException is appropriate for handling IO errors during XML parsing with XMLBeautifier.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java
📚 Learning: 2025-08-22T15:02:24.810Z
Learnt from: predic8
Repo: membrane/api-gateway PR: 2068
File: core/src/main/java/com/predic8/membrane/core/util/TextUtil.java:119-128
Timestamp: 2025-08-22T15:02:24.810Z
Learning: XMLBeautifier.parse(InputStream) throws Exception, not XMLStreamException specifically, so catching IOException in TextUtil.formatXML is appropriate for handling IO errors during XML parsing.
Applied to files:
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java (1)
24-40: Fix the invalid import (compilation blocker).
import com.predic8.membrane.core.lang.ExchangeExpression.*;is not a legal Java import (on-demand imports can only target packages). As a result,Languageis still unresolved and the class no longer compiles. Please remove this line and import the nested enum explicitly.-import com.predic8.membrane.core.lang.*; -import com.predic8.membrane.core.lang.ExchangeExpression.*; +import com.predic8.membrane.core.lang.*; +import com.predic8.membrane.core.lang.ExchangeExpression.Language;core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java (1)
23-40: Correct the ExchangeExpression import to restore compilation.
Same issue here:import com.predic8.membrane.core.lang.ExchangeExpression.*;is an invalid non-static import, so the nestedLanguagetype is still missing and the file will not compile. Please replace it with an explicit import of the nested enum.-import com.predic8.membrane.core.interceptor.lang.*; -import com.predic8.membrane.core.lang.*; -import com.predic8.membrane.core.lang.ExchangeExpression.*; +import com.predic8.membrane.core.interceptor.lang.*; +import com.predic8.membrane.core.lang.*; +import com.predic8.membrane.core.lang.ExchangeExpression.Language;core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
88-105: Remove merge conflict fromparseTokens().The parser still includes
<<<<<<< HEAD/=======/>>>>>>>. This prevents compilation and leaves ambiguity about which factory to use. Please resolve it and wire the tokens to the new factory:-<<<<<<< HEAD - tokens.add(new Expression(ExchangeExpression.newInstance(interceptor, language, expr))); -======= - tokens.add(new Expression(expression(router, language, expr))); ->>>>>>> f78bb2c5a6937831602bd693024f03f9f2acad2d + tokens.add(new Expression(ExchangeExpression.newInstance(interceptor, language, expr)));core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java (1)
30-61: Resolve both merge conflicts (imports andinit()).This file still contains conflict markers in the import block and inside
init(), so it will not compile. Please settle the conflicts and keep the Interceptor-based expression initialization:-import static com.predic8.membrane.core.interceptor.Outcome.ABORT; -<<<<<<< HEAD -======= -import static com.predic8.membrane.core.interceptor.Outcome.CONTINUE; -import static com.predic8.membrane.core.lang.ExchangeExpression.Language.SPEL; -import static com.predic8.membrane.core.lang.ExchangeExpression.expression; ->>>>>>> f78bb2c5a6937831602bd693024f03f9f2acad2d +import static com.predic8.membrane.core.interceptor.Outcome.ABORT; +import static com.predic8.membrane.core.interceptor.Outcome.CONTINUE; @@ -<<<<<<< HEAD - exchangeExpression = ExchangeExpression.newInstance(this, language, key); -======= - exchangeExpression = expression(router, language, key); ->>>>>>> f78bb2c5a6937831602bd693024f03f9f2acad2d + exchangeExpression = ExchangeExpression.newInstance(this, language, key);Also remove the obsolete static import for
ExchangeExpression.expressionif the helper no longer exists.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/ForInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java(4 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java(4 hunks)core/src/test/java/com/predic8/membrane/core/lang/AbstractExchangeExpressionTest.java(2 hunks)core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/test/java/com/predic8/membrane/core/lang/AbstractExchangeExpressionTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-369)
core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java (1)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java (1)
AbstractLanguageInterceptor(23-57)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/ForInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java(4 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java(4 hunks)core/src/test/java/com/predic8/membrane/core/lang/AbstractExchangeExpressionTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpressionTest.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- core/src/main/java/com/predic8/membrane/core/interceptor/flow/ForInterceptor.java
- core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java
- core/src/test/java/com/predic8/membrane/core/lang/AbstractExchangeExpressionTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/ratelimit/RateLimitInterceptor.java
- core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java
- core/src/test/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpressionTest.java
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (3)
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
MCElement(49-112)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java (1)
MCElement(42-150)core/src/main/java/com/predic8/membrane/core/interceptor/lang/Namespaces.java (2)
MCElement(24-102)MCElement(46-72)
core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/Request.java (1)
Request(32-369)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Analyze (java)
- GitHub Check: Automated tests
🔇 Additional comments (7)
core/src/main/java/com/predic8/membrane/core/interceptor/idempotency/IdempotencyInterceptor.java (2)
43-43: LGTM! Architectural change enables namespace support.Extending
AbstractLanguageInterceptoris the appropriate refactoring to enable XML namespace support for XPath expressions, aligning with the PR objectives. This eliminates the need for a locallanguagefield and integrates namespace propagation through the parent class.
50-59: Expression initialization correctly refactored for namespace support.The change from
expression(router, language, key)toexpression(this, language, key)enables namespace-aware expression construction by passing the interceptor (which implementsXMLNamespaceSupportthroughAbstractLanguageInterceptor). The inheritedlanguagefield is safely initialized at the class level with default valueSPELinAbstractLanguageInterceptor, ensuring it's available when used on line 54.core/src/main/java/com/predic8/membrane/core/interceptor/balancer/PolyglotSessionIdExtractor.java (1)
36-36: Adapter context update aligns with new namespace flow.Switching to
InterceptorAdapterkeeps this extractor in step with the namespace-aware expression pipeline while preserving existing behavior. Nicely done.core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (1)
85-92: Namespaces hook mirrors other interceptors.Bringing
CaseontoXMLNamespaceSupportwith thenamespaceschild element keeps XPath cases consistent with the broader namespace story. Looks spot on.core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
59-107: Init path now honors optional namespaces.By funnelling initialization through
InterceptorAdapter(router, namespaces), the extractor cleanly picks up namespace bindings without regressing existing configurations. All good here.core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
41-52: Template tokens now share interceptor context.Passing the interceptor through
newInstanceand intoparseTokenskeeps embedded expressions on the same namespace-aware footing as their parents. Thanks for tightening that integration.core/src/test/java/com/predic8/membrane/core/lang/jsonpath/JsonpathExchangeExpressionTest.java (1)
120-138: Tests cover adapter-based expression construction.Updating the JSONPath tests to build expressions through
InterceptorAdapter(router)keeps the suite aligned with the production factory changes. Great to see the coverage adjusted.
|
This pull request needs "/ok-to-test" from an authorized committer. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/test/java/com/predic8/membrane/core/lang/xpath/SetPropertyInterceptorXPathTest.java (1)
34-34: Remove unused field.The
ObjectMapper omfield is declared but never used in any of the tests.Apply this diff to remove the unused field:
- private ObjectMapper om = new ObjectMapper(); private Exchange exc;distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (2)
123-144: Strengthen the assertion for the no-namespace test case.The assertion
containsString("from")on line 142 is too weak and could match unintended responses (e.g., error messages like "Response from server"). Consider asserting the exact expected output when namespace-aware XPath fails to match, or at minimum use a more specific substring that uniquely identifies the expected behavior.For example:
- .body(containsString("from")); + .body(equalTo("Expected output when namespaces don't match"));Or document the expected behavior and assert a more specific pattern that distinguishes success from other "from"-containing responses.
31-190: Consider extracting common test structure into a helper method.All test methods follow an identical pattern with only the XML body and expected response varying. Extracting this into a helper method would reduce duplication and improve maintainability.
Example refactor:
private void assertNamespaceExtraction(String xmlBody, String expectedResponseSubstring) { // @formatter:off given() .contentType(TEXT_XML) .body(xmlBody) .post("http://localhost:2000") .then() .log().ifValidationFails(ALL) .statusCode(200) .body(containsString(expectedResponseSubstring)); // @formatter:on } @Test void namespaceAwareXPathExtraction() { String xmlBody = """ <per:person id="77" xmlns:per="https://predic8.de/person"> <per:name>Hans</per:name> <ns1:address xmlns:ns1="https://predic8.de/address"> <ns1:city>Cologne</ns1:city> </ns1:address> </per:person> """; assertNamespaceExtraction(xmlBody, "Hans from Cologne"); }This would make the tests more concise while maintaining clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
core/src/test/java/com/predic8/membrane/core/lang/xpath/SetPropertyInterceptorXPathTest.java(1 hunks)distribution/conf/convert-request-to-new-version.xslt(0 hunks)distribution/conf/convert-response-to-old-version.xslt(0 hunks)distribution/conf/security-api-v1.yml(0 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java(1 hunks)
💤 Files with no reviewable changes (3)
- distribution/conf/convert-request-to-new-version.xslt
- distribution/conf/security-api-v1.yml
- distribution/conf/convert-response-to-old-version.xslt
🧰 Additional context used
🧬 Code graph analysis (1)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
core/src/test/java/com/predic8/membrane/core/lang/xpath/SetPropertyInterceptorXPathTest.java (2)
37-107: Excellent test coverage and structure.The test organization effectively demonstrates namespace handling in XPath:
- Basic scenario without namespaces establishes the baseline
- Multiple strategies for namespaced XML (local-name(), namespace-uri(), proper prefix declarations)
- Clear comparison between incorrect usage (prefix without declaration) and correct usage (with namespace configuration)
- Comprehensive error handling validation
The progression of test cases provides good documentation of how to work with namespaces in XPath.
99-106: Thorough error handling validation.The test properly validates multiple aspects of error handling:
- Control flow (ABORT outcome)
- HTTP semantics (500 status)
- User experience (error message contains both the problematic expression and identifies the unknown prefix)
This ensures users receive actionable error feedback when namespace prefixes are not properly configured.
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (2)
24-29: LGTM! Well-structured test class setup.The test class properly extends the sample test base class and correctly references the example directory for namespace testing.
31-121: Excellent coverage of positive namespace scenarios.The test cases comprehensively validate namespace-aware XPath extraction across multiple scenarios: basic prefixed namespaces, data variations, different prefix bindings, and default namespaces. The assertions are specific and appropriate for integration testing.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (2)
51-51: Consider more specific assertions for stronger validation.The current assertions using
containsString()work well for integration tests and are less brittle. However, if you want more precise validation, you could assert the exact response body format or structure.For example:
.body(equalTo("Hans from Cologne")) // or use XPath/XML matchers if the response is XMLThis is optional—the current approach is acceptable for example tests.
Also applies to: 74-74, 97-97, 120-120, 144-144, 168-168
32-170: Optional: Extract helper method to reduce duplication.If you anticipate adding more test cases, consider extracting a helper method to reduce duplication:
private void assertNamespaceExtraction(String xmlBody, String expectedBodyContent) { given() .contentType(TEXT_XML) .body(xmlBody) .post("http://localhost:2000") .then() .log().ifValidationFails(ALL) .statusCode(200) .body(containsString(expectedBodyContent)); }However, the current self-contained approach is perfectly acceptable for test code clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (2)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (2)
25-30: Excellent test class structure.The class correctly extends
AbstractSampleMembraneStartStopTestcaseand configures the example directory. This will ensure the Membrane API Gateway starts with the XML namespaces example configuration before running tests.
32-170: Comprehensive test coverage of XML namespace functionality.The test suite thoroughly validates the PR's core feature with excellent scenario coverage:
- ✅ Basic namespace-aware XPath extraction
- ✅ Different XML prefixes with the same namespace URIs (proving URI-based matching)
- ✅ Default namespace handling
- ✅ Graceful degradation when namespace URIs don't match
- ✅ XPath attribute extraction to response headers
Each test is self-contained, follows the given-when-then pattern, and uses appropriate REST-assured assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (1)
125-146: Consider strengthening the assertion for clearer intent.The test correctly validates behavior with a mismatched namespace URI, but the assertion
containsString("Hans from")is somewhat weak—it doesn't explicitly verify that the city extraction failed.Consider one of these alternatives for clarity:
Option 1: Add a negative assertion
.statusCode(200) .body(containsString("Hans from")) + .body(not(containsString("Cologne")));Option 2: Check for empty/missing city explicitly
- .body(containsString("Hans from")); + .body(containsString("Hans from ")) // trailing space indicates missing cityChoose the option that matches the actual example behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
distribution/examples/xml/namespaces/proxies.xml(1 hunks)distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- distribution/examples/xml/namespaces/proxies.xml
🧰 Additional context used
🧬 Code graph analysis (1)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (1)
core/src/main/java/com/predic8/membrane/core/http/MimeType.java (1)
MimeType(31-214)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
distribution/src/test/java/com/predic8/membrane/examples/withoutinternet/test/xml/namespaces/NamespacesExampleTest.java (4)
1-30: Well-structured test setup.The class correctly extends the example test base class and properly configures the example directory. Imports are clean and follow established patterns.
32-76: Solid baseline tests for namespace-aware XPath.These tests establish the happy path for namespace-aware XML processing. Testing with two different datasets (Hans/Cologne and Maria/Berlin) confirms consistent behavior.
78-122: Excellent coverage of namespace edge cases.These tests validate critical namespace behaviors:
differentPrefixesStillMatchconfirms namespace URIs are matched, not prefix names (essential for interoperability)defaultNamespaceOnPersonensures default namespace handling works correctly (common in real-world XML)
148-170: Good coverage of attribute extraction.This test validates XPath attribute extraction (the
@idsyntax) and confirms the value flows correctly from XML attribute to response header. Nice complement to the element-content extraction in other tests.
…path-expressions' into xml-namespace-declarations-for-xpath-expressions
…path-expressions' into xml-namespace-declarations-for-xpath-expressions
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java (1)
27-27: Initialize namespaces list to prevent NullPointerException.The
namespacesfield is not initialized and will remain null ifsetNamespace()is never called. This causes NPE inNamespaceContextImplmethods (lines 78, 87, 96) that callnamespaces.stream().Apply this diff to initialize the field:
- private List<Namespace> namespaces; + private List<Namespace> namespaces = new ArrayList<>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
core/src/main/java/com/predic8/membrane/core/config/XMLElement.java(1 hunks)core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java(1 hunks)core/src/main/java/com/predic8/membrane/core/config/xml/XmlConfig.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/XMLSupport.java(1 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java(3 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java(4 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java(2 hunks)core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java(2 hunks)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java(5 hunks)core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java(4 hunks)core/src/test/java/com/predic8/membrane/core/interceptor/lang/NamespacesTest.java(1 hunks)core/src/test/java/com/predic8/membrane/core/lang/xpath/SetPropertyInterceptorXPathTest.java(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/src/main/java/com/predic8/membrane/core/config/XMLElement.java
🚧 Files skipped from review as they are similar to previous changes (3)
- core/src/test/java/com/predic8/membrane/core/lang/xpath/SetPropertyInterceptorXPathTest.java
- core/src/test/java/com/predic8/membrane/core/interceptor/lang/NamespacesTest.java
- core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java
🧰 Additional context used
🧬 Code graph analysis (4)
core/src/main/java/com/predic8/membrane/core/config/xml/XmlConfig.java (6)
core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java (2)
MCElement(24-102)MCElement(46-72)core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
MCElement(50-115)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java (1)
MCElement(41-151)core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (1)
MCElement(29-95)core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java (2)
MCElement(47-257)MCElement(230-242)core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (1)
MCElement(100-253)
core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java (5)
core/src/main/java/com/predic8/membrane/core/config/xml/XmlConfig.java (1)
MCElement(5-18)core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
MCElement(50-115)core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java (1)
MCElement(41-151)core/src/main/java/com/predic8/membrane/core/interceptor/flow/choice/Case.java (1)
MCElement(29-95)core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (1)
MCElement(100-253)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (2)
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java (1)
HardenedXmlParser(30-99)core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
XMLUtil(30-85)
core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (1)
core/src/main/java/com/predic8/membrane/core/lang/TemplateExchangeExpression.java (1)
TemplateExchangeExpression(30-167)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (24)
core/src/main/java/com/predic8/membrane/core/interceptor/lang/AbstractLanguageInterceptor.java (3)
24-24: LGTM! Clean interface implementation.The addition of
XMLSupportto the class signature is appropriate for enabling XML configuration across language interceptors.
46-59: LGTM! Well-documented XML configuration support.The getter and setter methods are properly annotated and documented. The
allowForeign = trueparameter on@MCChildElementcorrectly allows foreign XML elements, andorder = 10ensures proper element ordering.
30-30: Null-safety concern already addressed in code.Verification confirmed that the
xmlConfigfield is properly guarded with null checks. The only direct access to xmlConfig properties (XPathExchangeExpression.java:90) is already protected by a conditional check on line 89:if (xmlConfig != null). Subclasses do not directly access the field unsafely—they delegate to expression methods which handle null-safety internally. No additional safeguards are required.core/src/main/java/com/predic8/membrane/core/interceptor/XMLSupport.java (1)
19-24: LGTM! Clean and minimal interface design.The
XMLSupportinterface provides a clear contract for XML configuration propagation across interceptors. The design is simple and follows Java interface best practices.core/src/main/java/com/predic8/membrane/core/interceptor/flow/IfInterceptor.java (3)
42-42: LGTM! Consistent XMLSupport implementation.The addition of
XMLSupportinterface enables XML namespace configuration for XPath expressions in if conditions.
58-58: LGTM! Correct context propagation.The expression initialization now uses
thisinstead ofrouter, which allows the XML configuration to be propagated through theXMLSupportinterface. This is the correct approach for enabling namespace-aware XPath expressions.
137-150: LGTM! Well-documented XML configuration support.The XML configuration methods are properly annotated and documented, consistent with the pattern used in other interceptors.
core/src/main/java/com/predic8/membrane/core/proxies/AbstractServiceProxy.java (3)
101-101: LGTM! Target class gains XML configuration support.The implementation of
XMLSupportenables XML namespace configuration for URL expressions in target definitions.
114-119: LGTM! Proper XML config propagation through InterceptorAdapter.The expression initialization correctly uses
InterceptorAdapter(router, xmlConfig)to propagate XML configuration to template expressions. The null check onurlensures the adapter is only created when needed.
239-252: LGTM! Consistent XML configuration implementation.The XML configuration methods follow the established pattern with proper annotations and documentation.
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (3)
36-36: LGTM! Secure XML parser usage.Using the singleton
HardenedXmlParser.getInstance()ensures secure XML parsing with XXE protection across all XPath evaluations.
43-49: LGTM! Clean XML config extraction.The constructor properly extracts XML configuration from interceptors that implement
XMLSupportusing pattern matching, which is a clean approach.
96-98: LGTM! Runtime configurability added.The
setXmlConfigmethod allows dynamic configuration of XML namespaces, which is useful for runtime adjustments.core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.java (3)
48-48: LGTM! APIProxy gains XML configuration support.The implementation of
XMLSupportenables XML namespace configuration for test expressions in API proxies.
86-91: LGTM! Proper XML config propagation.The expression initialization correctly uses
InterceptorAdapter(router, xmlConfig)and is properly guarded by a null/empty check on thetestattribute.
244-256: LGTM! Consistent XML configuration implementation.The XML configuration methods follow the established pattern across all interceptors.
core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (3)
51-51: LGTM! XML configuration support added to deprecated extractor.While this class is deprecated, the
XMLSupportimplementation ensures backward compatibility for existing users who may need XML namespace support in their expressions.
59-61: LGTM! Correct XML config propagation.The expression initialization properly uses
InterceptorAdapter(router, xmlConfig)to enable XML namespace support.
101-114: LGTM! Well-documented XML configuration methods.The implementation is consistent with other interceptors, with proper documentation explaining the purpose of XML configuration.
core/src/main/java/com/predic8/membrane/core/lang/ExchangeExpression.java (3)
17-19: LGTM!The new imports support the InterceptorAdapter class and XMLSupport functionality introduced in this file.
53-68: Factory method signature updated to accept Interceptor.The change from
RoutertoInterceptorparameter enables XML namespace context propagation to GROOVY and XPATH expressions, aligning with the PR's objective. The added Javadoc appropriately guides callers to useInterceptorAdapterwhen no interceptor is available.Note: A reviewer questioned whether the interceptor is becoming a "god-object" and suggested declaring namespaces globally (see rrayst's comment on line 61). While this is a valid architectural concern, the current implementation is functionally correct.
70-100: No issues found. Code is correct.The
routerfield inAbstractInterceptoris declared asprotected, allowingInterceptorAdapter(a subclass) to safely access and assign to it. The pattern is consistent and will compile without issues.core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java (2)
46-72: LGTM!The
Namespaceinner class provides a clean data holder for prefix-URI mappings with appropriate annotations for XML configuration parsing.
74-101: NamespaceContext implementation is correct once line 27 is fixed.The
NamespaceContextImplcorrectly implements thejavax.xml.namespace.NamespaceContextcontract, returning appropriate defaults (NULL_NS_URI,null, or empty iterator) for unknown mappings. However, all methods will throw NPE until thenamespacesfield is initialized on line 27.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java (1)
27-27: Initialize namespaces field to prevent NullPointerException.The
namespacesfield remains uninitialized and will cause NPE inNamespaceContextImplmethods (lines 78, 87, 96) that callnamespaces.stream()wheneversetNamespace()is not called or is called with null.Apply this diff to initialize the field:
- private List<Namespace> namespaces; + private List<Namespace> namespaces = new ArrayList<>();
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java (1)
46-72: Consider making fields private for better encapsulation.The
prefixandurifields are public (lines 49-50), allowing direct access that bypasses the setters. While this may be intentional for framework compatibility, making them private would improve encapsulation and ensure all access goes through the annotated setters.Apply this diff if stricter encapsulation is desired:
- public String prefix; - public String uri; + private String prefix; + private String uri;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java(1 hunks)core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java (2)
core/src/main/java/com/predic8/membrane/core/config/xml/XmlConfig.java (1)
MCElement(5-18)core/src/main/java/com/predic8/membrane/core/interceptor/apikey/extractors/ApiKeyExpressionExtractor.java (1)
MCElement(50-115)
core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (2)
core/src/main/java/com/predic8/membrane/core/util/xml/parser/HardenedXmlParser.java (1)
HardenedXmlParser(30-99)core/src/main/java/com/predic8/membrane/core/util/xml/XMLUtil.java (1)
XMLUtil(30-85)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (5)
core/src/main/java/com/predic8/membrane/core/config/xml/Namespaces.java (4)
30-32: LGTM!The getter correctly exposes the NamespaceContext implementation for XPath expression evaluation.
34-40: Javadoc correction applied; verify null-safety.The Javadoc has been correctly updated per previous feedback. However, the setter accepts and assigns a potentially null list without validation. Combined with the uninitialized field on line 27, ensure that calling code never passes null, or add defensive null handling.
42-44: LGTM!The getter is correctly implemented. Returns null when namespaces are not configured, which may be the intended behavior for optional XML namespace configuration.
74-101: Implementation logic is correct but depends on null-safety fix.The NamespaceContext implementation correctly follows the contract:
getNamespaceURIreturnsNULL_NS_URIfor unmatched prefixesgetPrefixreturns null for unmatched URIsgetPrefixesreturns an iterator over all matching prefixesHowever, lines 78, 87, and 96 will throw NPE if
namespacesis null. This issue will be resolved once the critical initialization fix on line 27 is applied.core/src/main/java/com/predic8/membrane/core/lang/xpath/XPathExchangeExpression.java (1)
89-91: Thanks for adding the namespace null guard.This prevents the
XPathnamespace context setup from throwing whenXmlConfighas no namespaces configured.
3 participants
Summary by CodeRabbit
New Features
Documentation
Chores
Tests